Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Chromium .clang-format file, and apply it #115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AndrewScheidecker
Copy link
Contributor

As requested here, this adds a .clang-format that only contains BasedOnStyle: Chromium, and applies it to *.c, *.cc, *.h, and *.hh, so folks can see what the impact is.

@sbc100
Copy link
Member

sbc100 commented Nov 1, 2019

Just looking at the main wasm.h I think its looks much better after this change. I especially like the macro blocks with the trailing line continuations all the way to right.

I think yon can revert all the v8 stuff as hopefully we can delete all that stuff anyway now that v8 has upstream support.

@rossberg
Copy link
Member

rossberg commented Nov 1, 2019

It could be worse (we don't have many #if's :) ). I can easily live with most of its effects but three issues:

  1. It removes the spaces around function arrows everywhere, presumably because it confuses them with member dereferences. That's just broken.

  2. It does not respect single-line vs multi-line definition choices, but decides to reformat everything based on purely local metrics, which often destroys visual grouping and cohesion for sequences of related definitions.

  3. It attempts vertical alignment of inner parameters. That creates arbitrary jitter indentation, which is visually distracting and aesthetically displeasing, tends to waste space and/or creates reformatting churn, and it only works for fixed-width fonts. Vertical alignment is bogus except for leading spaces. That's a pet peeve of mine. :)

Is there a way to fix these? Last time I played with clang-format I couldn't figure it out. Some of these are just aesthetics, but for readable interface files at least it kind of matters some.

@sbc100, I'd prefer to keep the V8 prototype impl around for the time being, since it enables experimenting with extensions and changes to the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants